fix(security): disable docker-postgres on kyber - #1506
Conversation
…lures - Extract inline writeShellScript in secure-dotenv to external .sh file - Fix nixfmt drift in ollama/default.nix (multi-line args) - Fix shfmt drift in clipboard_copy_spec.sh (here-string spacing) - Switch Docker cache from GHA to registry-based to avoid auth timeout
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughUpdated Docker build caching to use GHCR registry-backed layer caches per architecture, refactored secure-dotenv module to use external shell script with Bash invocation, adjusted service enablement conditions, and added comprehensive test coverage for the secure-dotenv functionality. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…r reinfection Postgres container was compromised via public 0.0.0.0:5432 binding with trust auth. Invert the enable condition so docker-postgres only runs on galactica and matic.
Mesa DescriptionTL;DRDisabled What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the secure-dotenv module by extracting its shell logic into a separate script file and using pkgs.replaceVars for path substitution. It also corrects the enablement logic for the docker-postgres service and adds a comprehensive test suite for the new script. Feedback was provided to improve the robustness of the file-finding logic in the shell script by using null delimiters to safely handle filenames with special characters.
| @find@ "${HOME_DIR}" \ | ||
| -maxdepth 4 \ | ||
| \( -name '.env' -o -name '.env.*' -o -name '*.env' \) \ | ||
| 2>/dev/null | while IFS= read -r f; do | ||
| if [ -f "$f" ] && [ ! -L "$f" ]; then | ||
| current=$(@stat@ -c '%a' "$f") | ||
| if [ "$current" != "600" ]; then | ||
| chmod 600 "$f" | ||
| fi | ||
| fi | ||
| done |
There was a problem hiding this comment.
To robustly handle filenames that might contain spaces or newlines, it is recommended to use -print0 with find and read -d ''. This follows the organization's general rule for robustly parsing command output. Additionally, using -type f in the find command is more efficient and allows removing the manual file type check inside the loop.
| @find@ "${HOME_DIR}" \ | |
| -maxdepth 4 \ | |
| \( -name '.env' -o -name '.env.*' -o -name '*.env' \) \ | |
| 2>/dev/null | while IFS= read -r f; do | |
| if [ -f "$f" ] && [ ! -L "$f" ]; then | |
| current=$(@stat@ -c '%a' "$f") | |
| if [ "$current" != "600" ]; then | |
| chmod 600 "$f" | |
| fi | |
| fi | |
| done | |
| @find@ "${HOME_DIR}" \ | |
| -maxdepth 4 \ | |
| -type f \ | |
| \( -name '.env' -o -name '.env.*' -o -name '*.env' \) \ | |
| -print0 2>/dev/null | while IFS= read -r -d '' f; do | |
| current=$(@stat@ -c '%a' "$f") | |
| if [ "$current" != "600" ]; then | |
| chmod 600 "$f" | |
| fi | |
| done |
References
- To robustly parse command output in shell scripts, use a unique delimiter (e.g., tab) in the format string and read with a matching IFS. This is safer than splitting by spaces with cut, especially when data fields might contain spaces.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="spec/secure_dotenv_spec.sh">
<violation number="1" location="spec/secure_dotenv_spec.sh:82">
P2: The `.env` permission test only checks exit status and never asserts the mode changed to `600`, so it can pass even when behavior is broken.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| After 'cleanup' | ||
|
|
||
| It 'changes .env from 644 to 600' | ||
| When run bash "$PROCESSED_SCRIPT" "$TEST_HOME" |
There was a problem hiding this comment.
P2: The .env permission test only checks exit status and never asserts the mode changed to 600, so it can pass even when behavior is broken.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/secure_dotenv_spec.sh, line 82:
<comment>The `.env` permission test only checks exit status and never asserts the mode changed to `600`, so it can pass even when behavior is broken.</comment>
<file context>
@@ -0,0 +1,114 @@
+After 'cleanup'
+
+It 'changes .env from 644 to 600'
+When run bash "$PROCESSED_SCRIPT" "$TEST_HOME"
+The status should be success
+End
</file context>
1 participant
Summary
Test plan
Summary by cubic
Disable
docker-postgreson kyber to prevent reinfection; the service now runs only on galactica and matic. Also extractedsecure-dotenvinto a script with tests and switched CI Docker cache to the registry for stability.Bug Fixes
home-manager/services/docker-postgres: enabled only on galactica/matic.Refactors
secure-dotenvlogic tohome-manager/modules/secure-dotenv/secure-dotenv.shand call it from activation with the home dir; uses Nix var substitution forfind/stat.spec/secure_dotenv_spec.shand coverage checks; minor shfmt/nixfmt fixes and small cleanups (ollamaargs, mempalace module)..github/workflows/docker.ymlcache from GHA to registry-basedcache-from/cache-toto avoid auth timeouts.Written for commit 83525b0. Summary will update on new commits.